Handles more errors, expands Response class#14
Handles more errors, expands Response class#14dommccarty wants to merge 62 commits intoCodeMonkeysRu:masterfrom
Conversation
iVariable
left a comment
iVariable
left a comment
There was a problem hiding this comment.
Hello!
Thanks for the PR! Please check comments that I left.
I really like that u've added must-retry, wait-seconds and response headers 👍
But there are also several things which are questionable to me like:
- having 2 more or less same errors (server-side)
- helpers like
some* (IdsInvalid, IdsNew). Not totally convinced that they are useless, but also not sure that they are needed, as implementation is super trivial and all devs can write them outside - pls add tests for newly added methods
PS: also please make use of PSR-2 (http://www.php-fig.org/psr/psr-2/)
PPS: Also thanks for letting me know that this bundle is still in use :) I'll setup CI checks today for codestyle and tests and overall will do a cleanup
README.md
Outdated
|
|
||
| if ($response->existsInvalidDataKey()) { | ||
| //You used a reserved data key | ||
| $error_msg = 'Invalid data key in payload. ' . json_encode($message->getNotification()); |
README.md
Outdated
|
|
||
| if ($response->existsMismatchSenderId()) { | ||
| //A client sent the wrong senderId when it registered for pushes | ||
| $error_msg = 'Mismatch senderId. Problem clients are ' . json_encode($response->getMismatchSenderIdIds()); |
README.md
Outdated
|
|
||
| if ($response->existsInvalidDataKey()) { | ||
| //You used a reserved data key | ||
| $error_msg = 'Invalid data key in payload. ' . json_encode($message->getNotification()); |
README.md
Outdated
|
|
||
| if ($response->existsMismatchSenderId()) { | ||
| //A client sent the wrong senderId when it registered for pushes | ||
| $error_msg = 'Mismatch senderId. Problem clients are ' . json_encode($response->getMismatchSenderIdIds()); |
| const UNKNOWN_ERROR = 4; | ||
| const MALFORMED_RESPONSE = 5; | ||
| const INTERNAL_SERVER_ERROR = 6; | ||
| const SERVICE_UNAVAILABLE = 7; |
There was a problem hiding this comment.
Why do we need specific SERVICE_UNAVAILABLE? Is it that different from INTERNAL_SERVER_ERROR?
Do you have any specific actions needed to be taken from client perspective in case 1 error and in case of another?
There was a problem hiding this comment.
Agreed, removing. SERVICE_UNAVAILABLE seems more serious and we might want to shut off our push notifications for a while in that case. But someone can always just check the response headers now if they want to implement different handling.
library/CodeMonkeysRu/GCM/Sender.php
Outdated
| throw new Exception("Unknown error. ".$resultBody, Exception::UNKNOWN_ERROR); | ||
| break; | ||
|
|
||
| if ($resultHttpCode == "200") { |
There was a problem hiding this comment.
As I understood u've converted switch to if-elseif only for the sake of having "INTERNAL_ERROR" and "SERVICE_UNAVAILABLE".
Switch readability is a bit better, please convert if-elseif to switch(true) {} statement.
And speaking of those 2 "different" errors. Do we really need to distinguish between those 2 errors from client perspective? Ain't it enough to just know, that problem exists from "the other side". We can't really do anything about it in any case.
There was a problem hiding this comment.
I agree. I'll put it back to your version. Now that response headers are included in the Response object the programmer can do the more fine-grained analysis himself if he wants.
library/CodeMonkeysRu/GCM/Sender.php
Outdated
| else { | ||
| throw new Exception("Unknown error. ". $resultBody, Exception::UNKNOWN_ERROR); | ||
| } | ||
|
|
There was a problem hiding this comment.
Pls remove redundant new lines
library/CodeMonkeysRu/GCM/Sender.php
Outdated
| 'collapse_key' => 'getCollapseKey', | ||
| 'data' => 'getData', | ||
| 'notification' => 'getNotification', | ||
| 'notification' => 'getNotification', |
library/CodeMonkeysRu/GCM/Sender.php
Outdated
| return $data; | ||
| } | ||
|
|
||
library/CodeMonkeysRu/GCM/Sender.php
Outdated
|
|
||
| } | ||
|
|
||
| /* |
|
OK, tests added and passed! What do you think? |
|
GCM is currently responding to a request with an invalid data key with a HTTP response code 400, though in the documentation they say we should also be prepared to handle a response code 200 with the "error" on the individual response entries being "InvalidDataKey". Also they don't specify which response code they'll use if there's a Retry-After, but presumably it will be 503. In any case (haha), I changed it so if we get a Retry-After no Response object is created, because it's not necessary. So the Retry-After stuff has gone into the Exception. What do you think about these changes? |
|
Whoops, hadn't pushed that button before ... |
2 participants
This now checks for a lot more possible errors. Also expands the Response class to include the response headers, and some more metadata about the response.